fix(tmux): pass -u on the remaining attach paths - #1789
Conversation
|
👋 Thanks for the contribution — intake looks complete. Your PR body carries everything the maintainer's validation pipeline reads first: the problem, the reasoning, the human intent behind it, and an AI-disclosure. It will be applied, built, and tested against gate marker read: ai= |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughtmux session and control-client attachment commands now pass the global Changestmux UTF-8 attachment handling
Estimated code review effort: 2 (Simple) | ~15 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tmux/pty.go`:
- Around line 732-737: Document the controlling-TTY contracts near the
attach-client symbols: in internal/tmux/pty.go lines 732-737, explain that the
normal PTY attach requires a controlling terminal and describe its failure
behavior per `#1114`; in internal/tmux/controlpipe.go line 36, document the
control-pipe client’s distinct controlling-TTY behavior; and in
internal/tmux/keysender.go line 46, document the KeySender client’s
controlling-TTY behavior. Update comments only, preserving the existing command
implementations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ccb7d2f-ae5f-4bcd-81c4-9d34ea4f50d9
📒 Files selected for processing (6)
internal/tmux/attach_utf8_argv_lint_test.gointernal/tmux/control_mode_argv_lint_test.gointernal/tmux/controlpipe.gointernal/tmux/keysender.gointernal/tmux/pty.gointernal/tmux/pty_socket_test.go
What problem does this solve?
When agent-deck starts under a supervisor without a UTF-8 locale, four tmux attach clients outside the web terminal bridge can still start in non-UTF-8 mode and replace Unicode glyphs with
_. Fixes #1781 and completes the follow-up to #1779.Why this change
Pass tmux's global
-uoption beforeattach-sessionin the normal and read-only PTY attaches,ControlPipe, andKeySender. This makes each attach client use UTF-8 independently of its inherited locale. An AST regression test now rejects any production attach spawn that omits-uor places it after the subcommand; the existing exact-argv tests and comments were updated with the same contract.User impact
Terminal output on the remaining PTY and control-mode attach paths preserves Unicode when agent-deck is launched by locale-light environments such as launchd or systemd. Environments that already provide a UTF-8 locale keep the same behavior. The change adds no process, handshake, poll, or output-processing step.
Evidence
The merged #1779 records real output for the same tmux mechanism: without
-u, raw bytes for⏵⏵were5f 5f(__); with-u, they weree2 8f b5 e2 8f b5(⏵⏵). Issue #1781 identifies the four remaining attach sites that share that locale exposure.Test-first baseline / revert-check:
After adding
-u, the same command passes.Final checks:
The full sandboxed
go test ./...suite, runtime tmux reproduction, and before/after performance timing were not run: the local WSL environment has no Go toolchain, and runtime WSL execution was unavailable for this run. The performance-sensitive scope is limited to one extra global option on existing one-time attach process launches.AI disclosure
Model(s), if AI helped: gpt-5-codex
Prompt / session log (optional): Not provided.
What actually bothered you
My human asked: "完成一下 #1781" ("Complete issue #1781.")
Checklist
HOME=$(mktemp -d) XDG_CONFIG_HOME= XDG_DATA_HOME= XDG_CACHE_HOME= go test ./...- not run locally; see EvidenceSummary by CodeRabbit
Bug Fixes
/dev/ttyfor detached callers.Tests